Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rename: adjust all mount points #666

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gahr
Copy link

@gahr gahr commented Jan 22, 2024

Fixes #659

@yaazkal
Copy link
Collaborator

yaazkal commented Jul 8, 2024

Thanks @gahr for the PR. We had an issue (#698) related to devfs rules that is fixed now. Maybe that could be the root cause. We would appreciate it if you could test again to check if this PR is still needed.

@yaazkal yaazkal added the bug Something isn't working label Jul 8, 2024
@gahr
Copy link
Author

gahr commented Jul 9, 2024

Hi @yaazkal , how is this related to devfs rules? This is about mount point mounted via the MOUNT command in Bastillefiles that are not in fstab.

@yaazkal yaazkal requested a review from cedwards July 13, 2024 22:33
@yaazkal
Copy link
Collaborator

yaazkal commented Jul 14, 2024

Sorry @gahr that comment was intented for another PR which is now merged (multi tab in browser is bad for me). Let me take a look at your PR.

@yaazkal
Copy link
Collaborator

yaazkal commented Jul 14, 2024

Let me ask, did you test your PR with linux jails too?

@gahr
Copy link
Author

gahr commented Jul 15, 2024

No, I don't have any Linux jails

@yaazkal yaazkal added the help wanted Extra attention is needed label Jul 15, 2024
@yaazkal
Copy link
Collaborator

yaazkal commented Jul 15, 2024

Well, this needs extensive testing to not affect Midnight, Linux, or any other supported jails. Labeling the PR now to "help wanted" too.

@gahr
Copy link
Author

gahr commented Jul 15, 2024

@yaazkal why do you think this would be jail-type specific? This is the host's fstab, not the guest's.

@tschettervictor
Copy link
Contributor

Can we go over this again? I have many jails that I do renames on, and this is a problem for them.

@@ -81,24 +81,7 @@ update_fstab() {
# Update fstab to use the new name
FSTAB_CONFIG="${bastille_jailsdir}/${NEWNAME}/fstab"
if [ -f "${FSTAB_CONFIG}" ]; then
# Skip if fstab is empty, e.g newly created thick or clone jails
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing all these lines?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have much familiarity with this code, I just fixed it to fit my use case, so let's go through it.

My proposal is to change occurrences of ${bastille_jailsdir}/${TARGET} into ${bastille_jailsdir}/${NEWNAME} in the fstab file. I think this is hardly debatable: we are renaming the jail ${TARGET} into the jail ${NEWNAME}, so all mount points that were pointing to the former now need to point to the latter.

So why did I remove all other lines? I think the old code was trying to match a particular mount point into fstab, namely, the ..../root/.bastille mountpoint. The original bug is that it left all other custom mount points alone, unrenamed.

So, my take is to sed them all, which makes the rest of the code useless.

The linprocfs/linsysfs special case should also be covered, although I don't understand why the leading dot in the string matched in sed -i '' "s|.${bastille_jailsdir}/${TARGET}/|${bastille_jailsdir}/${NEWNAME}/|" "${FSTAB_CONFIG}".

Hope this helps clarify.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I see what you mean.
However, what happens if a jail is named the same as a directory in the path specified?

Say you have a directory mounted into a jail named test, and the jail is also named test?

What do you think would be the best way to solve this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a problem for the whole of the bastille project. Rename.sh and clone.sh use the approach you do to rename fstab and jail.conf files.

I suppose we could document that you should try to never name your jails in a similar manner as your directories.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would that match ${bastille_jailsdir}/${TARGET} or ${bastille_jailsdir}/${NEWNAME}? You'd have to have a path, say, /usr/local/bastille/jails/foo under bastlle_jailsdir, so your mount point could be:

/host/path /usr/local/bastille/jails/TARGET/usr/local/bastille/jails/TARGET nullfs ro 0 0

In this case, yes, my approach would rename both TARGET to NEW, while probably you only want to rename the first.

I'd say it's still safer than what we're doing now...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the only place it's called is here. I'm good with this change, unless @cedwards has a good reason to retain the original code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tschettervictor did you test this code on your system also??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still working on that...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My testing confirms that the old code does not rename the mount points at all.
However, I would keep the old code.

In the "clone.sh" function this line
sed -i '' "s|${bastille_jailsdir}/${TARGET}/root/|${bastille_jailsdir}/${NEWNAME}/root/|" "${FSTAB_CONFIG}"
is added, which is not present in the "rename.sh"

I would argue that we should just make the "rename" code identical to the "clone" code as they attempt to do the exact same thing.
The current code does in fact change the RELEASE mount point, but does not touch any other custom mount points.

Here is the code from the "clone.sh"

 update_fstab() {
    # Update fstab to use the new name
    FSTAB_CONFIG="${bastille_jailsdir}/${NEWNAME}/fstab"
    if [ -f "${FSTAB_CONFIG}" ]; then
        FSTAB_RELEASE=$(grep -owE '([1-9]{2,2})\.[0-9](-RELEASE|-RELEASE-i386|-RC[1-9]|-BETA[1-9]|-CURRENT)|([0-9]{1,2}(-stable-build-[0-9]{1,3}|-stable-LAST))|(current-build)-([0-9]{1,3})|(current-BUILD-LATEST)|([0-9]{1,2}-stable-BUILD-LATEST)' "${FSTAB_CONFIG}" | uniq)
        FSTAB_CURRENT=$(grep -w ".*/releases/.*/jails/${TARGET}/root/.bastille" "${FSTAB_CONFIG}")
        FSTAB_NEWCONF="${bastille_releasesdir}/${FSTAB_RELEASE} ${bastille_jailsdir}/${NEWNAME}/root/.bastille nullfs ro 0 0"
        if [ -n "${FSTAB_CURRENT}" ] && [ -n "${FSTAB_NEWCONF}" ]; then
            # If both variables are set, update as needed
            if ! grep -qw "${bastille_releasesdir}/${FSTAB_RELEASE}.*${bastille_jailsdir}/${NEWNAME}/root/.bastille" "${FSTAB_CONFIG}"; then
                sed -i '' "s|${FSTAB_CURRENT}|${FSTAB_NEWCONF}|" "${FSTAB_CONFIG}"
            fi
        fi
        # Update additional fstab paths with new jail path
        sed -i '' "s|${bastille_jailsdir}/${TARGET}/root/|${bastille_jailsdir}/${NEWNAME}/root/|" "${FSTAB_CONFIG}"
    fi
}

I would just add the last line here to the rename function, but keep the old code in place just in case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the same time, the original code was trying to do something as simple as rename the entire RELEASE fstab, when it is far easier to just rename all occurrences of the old jail to the new jail.

Unless @cedwards was trying to do something specific with it...

@tschettervictor
Copy link
Contributor

I've added PR #757 to also change the "update_fstab" function. Here it is for reference.

It indeed makes more sense to search the fstab file for all old jail paths, and simply rename them to the new jail paths.

update_fstab() {
    # Update fstab to use the new name
    FSTAB_CONFIG="${bastille_jailsdir}/${NEWNAME}/fstab"
    if [ -f "${FSTAB_CONFIG}" ]; then
        # Update fstab paths with new jail path
        sed -i '' "s|${bastille_jailsdir}/${TARGET}|${bastille_jailsdir}/${NEWNAME}|" "${FSTAB_CONFIG}"
    fi
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] rename doesn't consider custom MOUNT
4 participants